[Off-chain] feat: in-memory query cache(s) #994
Conversation
(cherry picked from commit 7d48aef23354497be55958ad2f484e1734550249)
There was a problem hiding this comment.
@bryanchriswhite I did a superficial review but did not dive into the validation of the business logic line-by-line.
Is there any section where you'd want another pair of 👀 ?
Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
bryanchriswhite
force-pushed
the
issues/543/cache/memory
branch
from
4ac0eef to
011d2d7
Compare
bryanchriswhite
force-pushed
the
issues/543/cache/memory
branch
from
011d2d7 to
d5ce62f
Compare
There was a problem hiding this comment.
Love the iteration. Thanks @bryanchriswhite!
Left a few new comments, a few replies to older threads, but it should be g2g after the next round 🙌
|
As discussed, reorganizing like so:
|
Co-authored-by: red-0ne <red-0ne@users.noreply.github.com>
Co-authored-by: red-0ne <red-0ne@users.noreply.github.com>
| Clear() | ||
| } | ||
|
|
||
| // HistoricalKeyValueCache extends KeyValueCache to support getting and setting values |
There was a problem hiding this comment.
HistoricalKeyValueCache no longer extend KeyValueCache :)
| // Get retrieves the value from the cache with the given key. If the cache is | ||
| // configured for historical mode, it will return the value at the latest **known** | ||
| // version, which is only updated on calls to SetAsOfVersion, and therefore is not | ||
| // guaranteed to be the current version w.r.t the blockchain. | ||
| func (c *keyValueCache[T]) Get(key string) (T, bool) { |
There was a problem hiding this comment.
| // Get retrieves the value from the cache with the given key. If the cache is | |
| // configured for historical mode, it will return the value at the latest **known** | |
| // version, which is only updated on calls to SetAsOfVersion, and therefore is not | |
| // guaranteed to be the current version w.r.t the blockchain. | |
| func (c *keyValueCache[T]) Get(key string) (T, bool) { | |
| // Get retrieves the value from the cache with the given key. | |
| func (c *keyValueCache[T]) Get(key string) (T, bool) { |
| } | ||
|
|
||
| // Set adds or updates the value in the cache for the given key. | ||
| func (c *keyValueCache[T]) Set(key string, value T) error { |
There was a problem hiding this comment.
Does it still make sense to return an error here?
|
|
||
| import "cosmossdk.io/errors" | ||
|
|
||
| const codesace = "client/query/cache" |
There was a problem hiding this comment.
| const codesace = "client/query/cache" | |
| const codespace = "client/cache" |
| const codesace = "client/query/cache" | ||
|
|
||
| var ( | ||
| ErrKeyValueCacheConfigValidation = errors.Register(codesace, 3, "invalid query cache config") |
There was a problem hiding this comment.
| ErrKeyValueCacheConfigValidation = errors.Register(codesace, 3, "invalid query cache config") | |
| ErrKeyValueCacheConfigValidation = errors.Register(codesace, 1, "invalid query cache config") |
|
|
||
| // getLatestVersionNumber returns the latest version number (not the value) of the given key. | ||
| func (c *historicalKeyValueCache[T]) getLatestVersionNumber(key string) int64 { | ||
| c.valuesMu.Lock() |
There was a problem hiding this comment.
Shouldn't this be a RLock, especially that it's used in GetLatestVersion?
| } | ||
|
|
||
| // Update sortedDescVersions and ensure the list is sorted in descending order. | ||
| if _, versionExists := valueHistory.versionToValueMap[version]; !versionExists { |
There was a problem hiding this comment.
Wdyt about returning an error if SetVersion tries to set a different value to an already existing historical value?
I don't think the following could be a valid use case:
cache.SetVersion(sameKey, value, sameVersion)
cache.SetVersion(sameKey, differentValue, sameVersion)
| } | ||
|
|
||
| // Validate ensures that the historicalKeyValueCacheConfig isn't configured with incompatible options. | ||
| func (cfg *historicalKeyValueCacheConfig) Validate() error { |
There was a problem hiding this comment.
Shouldn't historicalKeyValueCacheConfig#Validate also call keyValueCacheConfig#Validate ?
| // returns an error. | ||
| func (c *historicalKeyValueCache[T]) SetVersion(key string, value T, version int64) error { | ||
| // DEV_NOTE: MUST call getLatestVersionNumber() before locking valuesMu. | ||
| latestVersion := c.getLatestVersionNumber(key) |
There was a problem hiding this comment.
I think getLatestVersionNumber should be under the lock of the calling method.
In order to secure that the latest version is not changed between the getLatestVersionNumber call and usage of the latestVersion in the caller.
This would require getLatestVersionNumber itself to not call Lock or RLock.
It would also involve some refactoring for GetLatestVersion and GetVersion though.
| require.False(t, isCached) | ||
| }) | ||
|
|
||
| t.Run("historical cache ignores TTL expiration", func(t *testing.T) { |
There was a problem hiding this comment.
Historical cache is still subject to maxKeys FIFO eviction policy.
I believe it's worth test covering it.
pkg/client/query/cache/config.go
Outdated
| // value versions are pruned. | ||
| // E.g.: Given a latest version of 100, and a maxVersionAge of 10, then the | ||
| // oldest version that is not pruned is 90 (100 - 10). | ||
| // If 0, no historical pruning is performed. It ONLY applies when historical is true. |
There was a problem hiding this comment.
| // If 0, no historical pruning is performed. It ONLY applies when historical is true. | |
| // If 0, no historical pruning is performed. | |
| // ONLY applies when historical is true. |
pkg/client/query/cache/config.go
Outdated
| historical bool | ||
| // maxVersionAge is the max difference between the latest known version and | ||
| // any other version, below which value versions are retained, and above which | ||
| // value versions are pruned. |
There was a problem hiding this comment.
Are they pruned or just not considered cache hits? #PUC
If they're evicted, please make that explicit.
| @@ -0,0 +1,378 @@ | |||
| package cache | |||
There was a problem hiding this comment.
Can you a new page in the docs with the diagram you have?
No need to add text or detail, but just want to make sure the diagram is not forgotten and the foundation to add details in the future is in place.
pkg/client/query/cache/memory.go
Outdated
| // cacheValueHistory stores cachedValues by version number and maintains a sorted | ||
| // list of version numbers for which cached values exist. This list is sorted in | ||
| // descending order to improve performance characteristics by positively correlating | ||
| // index with age. |
There was a problem hiding this comment.
| // cacheValueHistory stores cachedValues by version number and maintains a sorted | |
| // list of version numbers for which cached values exist. This list is sorted in | |
| // descending order to improve performance characteristics by positively correlating | |
| // index with age. | |
| // cacheValueHistory maintains: | |
| // - Cached values indexed by version number | |
| // - A descending-sorted list of version numbers for existing cached values | |
| // | |
| // The descending sort order optimizes performance by correlating index with age. |
pkg/client/query/cache/memory.go
Outdated
| // Get retrieves the value from the cache with the given key. If the cache is | ||
| // configured for historical mode, it will return the value at the latest **known** | ||
| // version, which is only updated on calls to SetAsOfVersion, and therefore is not | ||
| // guaranteed to be the current version w.r.t the blockchain. |
There was a problem hiding this comment.
| // Get retrieves the value from the cache with the given key. If the cache is | |
| // configured for historical mode, it will return the value at the latest **known** | |
| // version, which is only updated on calls to SetAsOfVersion, and therefore is not | |
| // guaranteed to be the current version w.r.t the blockchain. | |
| // Get retrieves a value from the cache using the provided key. | |
| // | |
| // For historical mode: | |
| // - Returns the value at the latest known version | |
| // - Latest version is only updated via SetAsOfVersion | |
| // - No guarantee of returning current blockchain version |
pkg/client/query/cache/memory.go
Outdated
| // DEV_NOTE: Intentionally not pruning here to improve concurrent speed; | ||
| // otherwise, the read lock would be insufficient. The value will be | ||
| // overwritten by the next call to Set(). If usage is such that values | ||
| // aren't being subsequently set, maxKeys (if configured) will eventually | ||
| // cause the pruning of values with expired TTLs. |
There was a problem hiding this comment.
| // DEV_NOTE: Intentionally not pruning here to improve concurrent speed; | |
| // otherwise, the read lock would be insufficient. The value will be | |
| // overwritten by the next call to Set(). If usage is such that values | |
| // aren't being subsequently set, maxKeys (if configured) will eventually | |
| // cause the pruning of values with expired TTLs. | |
| // DEV_NOTE: Not pruning here to optimize concurrent speed: | |
| // - Read lock alone would be insufficient for pruning | |
| // - Next Set() call will overwrite the value | |
| // - If values aren't subsequently set, maxKeys config will eventually trigger | |
| // pruning of TTL-expired values |
Summary
Adds the
QueryCache[T any]andHistoricalQueryCache[T any]interfaces,InMemoryCache[T any]implementation, configurations, and options functions.--- title: Legend --- classDiagram-v2 class GenericInterface__T__any { <<interface>> GenericMethod() T } class Implemenetation { ExportedField FieldType unexportedField FieldType } Implemenetation --|> GenericInterface__T__any: implements %% class Embedder__T__any { %% <<interface>> %% GenericInterface[T] %% } %% Embedder__T__any ..|> GenericInterface__T__any: embeds--- title: Caches --- classDiagram-v2 class KeyValueCache__T__any { <<interface>> Get(key string) (value T, isCached bool) Set(key string, value T) (err error) Delete(key string) Clear() } class HistoricalKeyValueCache__T__any { <<interface>> GetLatestVersion(key string) (value T, isCached bool) GetVersion(key string, version int64) (value T, isCached bool) SetVersion(key string, value T, version int64) (err error) } class keyValueCache__T__any:::cacheImpl keyValueCache__T__any --|> KeyValueCache__T__any class historicalKeyValueCache__T__any historicalKeyValueCache__T__any --|> HistoricalKeyValueCache__T__anyIssue
Type of change
Select one or more from the following:
consensus-breakinglabel if so. See [Infra] Automatically add theconsensus-breakinglabel #791 for detailsTesting
make docusaurus_start; only needed if you make doc changesmake go_develop_and_testmake test_e2edevnet-test-e2elabel to the PR.Sanity Checklist